refactor(worker-javascript): extract Phase 3 helpers from Core.js#1039
Merged
dqnykamp merged 13 commits intoDoenet:mainfrom May 3, 2026
Merged
refactor(worker-javascript): extract Phase 3 helpers from Core.js#1039dqnykamp merged 13 commits intoDoenet:mainfrom
dqnykamp merged 13 commits intoDoenet:mainfrom
Conversation
Begin breaking up the 13,837-line Core class by lifting seven self- contained, low-coupling helpers into TypeScript modules. The pattern matches the existing composed siblings (Dependencies.js, ParameterStack): each module is constructed with a `core` back-reference, and Core retains a thin delegating wrapper for every method/property that was previously on the class so external callers (CoreWorker, tests, components, and `coreFunctions`-bound references) continue to work unchanged. Modules extracted: - DiagnosticsManager.ts — diagnostics queue + source-location walk - StateVariableNameResolver.ts — pure-function name resolution utilities - VisibilityTracker.ts — visibility state and save/suspend timers - StatePersistence.ts — save to localStorage / database - AutoSubmitManager.ts — debounced answer-submit queue - NavigationHandler.ts — handleNavigatingToComponent, navigateToTarget - ResolverAdapter.ts — adapter to the external Rust name resolver No behavior change. Core.js drops from 13,837 to 12,909 lines. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Continues the Core.js breakup with six moderate-effort modules. Same pattern as Phase 1 (composed sibling holding a `core` back-reference, thin delegating wrapper on Core for every public method/property). No behavior change. Modules extracted: - RendererInstructionBuilder.ts — owns componentsToRender, componentsWithChangedChildrenToRender, rendererState; the dast instruction stream sent to the renderer - ProcessQueue.ts — owns processQueue, processing, stopProcessingRequests; async request queue and entry-point scheduling (executeProcesses, requestAction, requestUpdate, requestRecordEvent) - ComponentLifecycle.ts — stateless: registration, ancestors, defining-child splicing, propagation to shadows - ChildMatcher.ts — child-group matching, adapter substitution, rendered-child filtering (recursion guard only) - DeletionEngine.ts — stateless two-phase component deletion - ActionTriggerScheduler.ts — owns stateVariableChangeTriggers, actionsChangedToActions, originsOfActionsChangedToActions; trigger polling and chained-action graph Core.js drops from 12,909 to 11,253 lines (this PR), 13,837 → 11,253 since the refactor began (~18.7%). Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
The deep guts of Core. Same composition pattern as Phases 1 and 2: each module is constructed with a `core` back-reference, and Core retains thin delegating wrappers for every public method/property. No behavior change. Modules extracted (in dependency order): - StateVariableDefinitionFactory.ts — synchronous shape-building: attribute / adapter / reference-shadow definitions, plus the shadow-conversion modifiers - StateVariableInitializer.ts — runtime initialization: lazy-resolving getters, dependency wiring, array-entry materialization, prop-index resolution - ComponentBuilder.ts — recursive component instantiation from serialized DAST plus the post-creation error-component flush - CompositeExpander.ts — composite expansion + replacement swap into active children + active/inactive marking; mutually recursive with ComponentBuilder via Core's delegators Subtle fix: `core.publicCaseInsensitiveAliasSubstitutions.bind(this)` calls inside CompositeExpander needed `bind(this.core)` — the wrapper on Core uses `this.componentInfoObjects`, so the bind target must be Core, not the manager. Core.js drops from 11,253 to 6,063 lines (this PR), 13,837 → 6,063 since the refactor began (~56.2%). Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
|
…ndings
Two regressions from the Phase 3 extraction, surfaced in CI:
1. StateVariableInitializer.ts:425: \`let core = this;\` captured the
manager instead of Core, so the five \`core.addDiagnostic({...})\`
calls inside the inner array-callbacks (set up by
\`initializeArrayStateVariable\` for setArrayValue / getArrayValue /
etc.) failed at runtime with "core.addDiagnostic is not a function".
Fix: \`let core = this.core;\`. Caught by functionoperators.test.ts.
2. StateVariableDefinitionFactory.ts:1299: inside
\`stateDef.returnArrayDependenciesByKey = function () {...}\`, \`this\`
resolves to the stateDef at call time (not the manager), and
\`stateDef.arrayVarNameFromArrayKey\` is the method to call. The
blanket \`this.\` → \`this.core.\` transform in the extraction script
incorrectly added \`core.\` here. Fix: revert to
\`this.arrayVarNameFromArrayKey(key)\`. Caught by spreadsheet,
curve, curve.bezier, odesystem, and rectangle tests.
Audited the rest of the extracted modules for the same pattern
(\`function () {...}\` callbacks attached to stateVarObj/stateDef with
\`this.core.X\` references inside): no further occurrences.
ComponentBuilder.ts and CompositeExpander.ts have no inner-callback
patterns at all.
Verified: all 91 tests across the failing CI files plus 238 broader
regression tests pass.
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
4 tasks
Brings in the squash-merged Phase 1 (Doenet#1036) and Phase 2 (Doenet#1038) PRs along with their post-review changes, and adopts the same review-driven patterns in the remaining Phase 3 modules. Phase 1/2 manager files (ActionTriggerScheduler, AutoSubmitManager, ChildMatcher, ComponentLifecycle, DeletionEngine, DiagnosticsManager, ProcessQueue, RendererInstructionBuilder, ResolverAdapter, StatePersistence, StateVariableNameResolver, VisibilityTracker) plus AGENTS.md, CLAUDE.md, and the new utils/timerErrors.ts and CORE_REFACTOR_DEFERRED.md are taken from upstream. Core.js merges Phase 3's extraction wrappers with upstream's Phase 1/2 review changes: - import * as nameResolver (replaces aliased named imports) - short-form `// → managerName` markers replace the long per-section comment blocks - top-of-class block comment now lists all phases - removed `set hasPendingDiagnostics` (callers go through markPending()) - replaced open-coded processQueueManager three-field reset with processQueueManager.reset() - added `// → componentBuilder`, `// → compositeExpander`, `// → stateVariableDefinitionFactory`, `// → stateVariableInitializer`, `// → resolverAdapter` markers for the new sections Phase 3 modules adopt the Phase 2 review conventions: - ComponentBuilder.ts: `core.hasPendingDiagnostics = true` → `core.diagnosticsManager.markPending()` (the setter on Core was removed in the Phase 1 review) - ComponentBuilder.ts and CompositeExpander.ts: standardize on `core._components` (was using the `core.components` getter for the same underlying array) CORE_REFACTOR_DEFERRED.md updates: - Type-the-`core: any` and stateless-managers items now list Phase 3 managers alongside Phase 1/2. - Pre-existing fire-and-forget Core.js line numbers updated for the post-Phase-3 layout (444, 4387, 483-486). - Carried-over TODO/XXX inventory now includes the Phase 3 markers in StateVariableInitializer, StateVariableDefinitionFactory, ComponentBuilder, and CompositeExpander. - _components/components convention note now covers Phase 3. Verification: - `tsc --noEmit` produces strictly fewer errors than the pre-merge backup (359 vs 365 — the removed setter and the `core._components` switch resolve a handful of `any` complaints; nothing new was introduced). - callAction.test.ts (20/20 — exercises StatePersistence + ProcessQueue + ActionTriggerScheduler), circle.test.ts and spreadsheet.test.ts (65 passed, 1 todo — exercises StateVariableInitializer's array-callback path that the Phase 3 follow-up commit fixed) all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four post-extraction cleanups surfaced in PR review: 1. Core.js: `addUndisplayableErrorChildrenToAncestor` wrapper now takes `(parent, undisplayableErrorChildren)` positional args, matching the manager's signature. The single-arg `(args)` form was a latent bug — no current external caller, but the next would have silently dropped the second argument. 2. Core.js: move `checkForActionChaining` wrapper next to the other `actionTriggerScheduler` delegators; it was misfiled inside the `// → stateVariableInitializer` block. 3. StateVariableInitializer.ts: rewrite class JSDoc to match the actual getter-binding behavior (capture-and-rely-on-Core's-eager-bind, not lazy reach-back), drop the bogus `core.dependencies` reference, and correct `actionTriggerScheduler` → `stateVariableChangeTriggers`. Notes the Phase 4 implication if `getStateVariableValue` later moves. 4. StateVariableDefinitionFactory.ts: drop `arrayVarNameFromArrayKey` from the Core back-reference list — it's invoked on the per-variable `stateDef`, not on Core (matches the `caf3033f5` fix at line 1299). No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Cleanup pass on the four extracted modules. No behavior change. ComponentBuilder.ts: - Drop ~10 comments that just restate the next line (`// create component itself`, `// add a new level to parameter stack;`, etc.). - Drop stale commented-out call sites (`setAncestors`, the `readOnlyProxyHandler` Proxy wrapping of `shadows`, the `collateCountersAndPropagateToAncestors` line, and two `console.timeLog` trace calls). - Drop the orphaned section header above `addQueuedErrorComponentsFromStateVariables` — that method is not a state-variable-definition delegator. - Rewrite the "in case component with same name was deleted" comment to fix typos in the variable names and lift it from one-liner to a real WHY explanation. CompositeExpander.ts: - Sweep ~25 commented-out `console.log` debug lines across the file (one of them referenced an undefined `composite.componentIdx`). - Tighten three multi-line WHAT comments at the top of `expandCompositesOfDescendants`, `expandCompositeOfDefiningChildren`, and `replaceCompositeChildren` into one-sentence summaries. - Fix typo "then its one replacement receive that name" → "should receive that name" in the `adjustForCreateComponentIdxName` JSDoc. - Remove a misleading comment that claimed the methods that follow are resolver-adapter delegators (they are not). StateVariableDefinitionFactory.ts: - Remove three identical inline `// Note: isAttribute is not accessed anywhere` comments. - Drop four commented-out `console.log` debug lines from the shadow `arrayDefinitionByKey` generator. StateVariableInitializer.ts: - Drop a 14-line block of commented-out alternative `value` getter implementations. - Add the missing blank line between `initializeStateVariable` and `initializeArrayEntryStateVariable`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Items surfaced during the Phase 3 PR review but intentionally not applied in this PR. Bundled here so the next agent can pick them up without re-deriving: - Four families of duplication in StateVariableDefinitionFactory (shadowingInstructions blocks ×3, stateVariableForAttributeValue resolution ×2, attribute definition/inverseDefinition closures ×2, attributesToCopy loop ×3). - Two near-mirror branches of ComponentBuilder.addComponents that share three drains worth of helpers. - Two duplicated patterns in CompositeExpander (the compositesBeingExpanded indexOf+splice "finished expanding" cleanup, and the parallel unexpandedCompositesReady/NotReady cleanup). - Dead `replacementsCreated` re-init guard in expandShadowingComposite. - Pre-existing bug: verifyReplacementsMatchSpecifiedType warnings loop iterates `diagnostics` twice instead of once-each for errors and warnings (carried over verbatim from Core.js). - Pre-existing bug: `!numDimensionsInArrayKey > stateVarObj.numDimensions` in initializeArrayStateVariable is always false, making a diagnostic unreachable (also pre-existing, since 2021). - Re-home recursivelyReplaceCompositesWithReplacements out of StateVariableInitializer (it walks composites, not state vars) and drop its unread forceExpandComposites parameter while moving. - Minor ComponentBuilder hygiene: loose equality on componentIdx, verbose nTimesAddedComponents increment-or-init, redundant console.error+rethrow. - Lift findShadowedChildInSerializedComponents into utils/ — pure recursion over a serialized tree, no Core reads/writes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Records the audit pattern that caught the two Phase 3 regressions fixed in caf3033, so future mechanical extractions of Core.js methods don't have to re-derive it. Three concrete patterns called out: `let core = this` captures, `function () {}` callbacks attached to plain objects, and `.bind(this)` on wrappers passed by reference. Surfaced in the Phase 3 PR review as a recurring trap class distinct from the concrete deferred cleanups already listed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 3 of breaking up
packages/doenetml-worker-javascript/src/Core.js. Phases 1 (#1036) and 2 (#1038) have landed onmainand this branch has merged them in, so the diff below is Phase 3 only. The deep guts — four highly-interconnected modules that contain the algorithmic heart of Core. Same composition pattern as the prior phases (composed sibling holding acoreback-reference; thin delegating wrapper on Core for every public method/property). No behavior change.Net effect over all three phases:
Core.jsdrops from 13,837 → 6,063 lines (-56.2%).Modules extracted in Phase 3
StateVariableDefinitionFactory.tsStateVariableInitializer.tsComponentBuilder.tsCompositeExpander.tsSequencing rationale
Per the multi-phase plan, the four modules were extracted in dependency order:
StateVariableDefinitionFactoryfirst (pure shape-building, smallest blast radius), thenStateVariableInitializerwhich consumes its output, thenComponentBuilderandCompositeExpandertogether — they're mutually recursive (creating components triggers expansion; expanding creates components), and the cross-calls go through Core's delegators rather than trying to invert the call graph.Subtle fix
The diagnostics test suite caught one bug mid-phase:
core.publicCaseInsensitiveAliasSubstitutions.bind(this)insideCompositeExpanderwas binding the wrapper to the manager instead of to Core, sothis.componentInfoObjectswas undefined when later invoked. Fixed tobind(this.core). This is exactly the class of issue the plan called out as a risk (R4) — wrappers passed by reference need their bind targets explicitly aimed at Core.A second instance of the same shape (
let core = this;carried verbatim into a manager method, where it should now readlet core = this.core;) was caught later byfunctionoperators,spreadsheet,curve,odesystem, andrectangletests and fixed incaf3033f5. The audit pattern that catches this whole class of regression is now documented inCORE_REFACTOR_DEFERRED.md("Audit class for future extraction phases:this-rebinding traps") so the next phase doesn't re-derive it.Test plan
npm run build -w @doenet/doenetml-worker-javascriptpasses after every extractionbindbug immediatelyComponentBuilder↔CompositeExpandercross-recursioncaf3033f5) was caught byfunctionoperators/spreadsheet/curve/odesystem/rectangle— outside the four buckets above. CI is the real safety net for this refactor's regression surface.🤖 Generated with Claude Code